This test was written to ensure that an invalid message NEVER triggers an unintentional fatal error, so it seems a bit weird to change it in this way.

Yeah I was unsure if it made more sense to just remove these test cases or just test for the sdk error. I guess now the property that we have is that invalid messages are always forwarded into the router, so we can either check for this error, get rid these tests, or write a Router interface that p2p.Router implements but maybe that's overkill.

since closed is atomic, could we move this lock to directly before
handler, exists := n.markRequestFulfilled(requestID)?

Should we move it to inside markRequestFulfilled instead?

Yeah that makes much more sense than what I was currently doing.

This is a very subtle change. So we should be terrified to make it.

I believe we must hold the lock when checking for n.closed on all of the inbound response + requestFailed flows to avoid a potential panic due to a write to a closed channel (or closing a channel twice).

As discussed offline I'll see if I can make a separate regression test for this invariant as a follow-up to this PR.

I documented the invariant on n.closed as well 👍

Are we okay with dropping responses (and timeouts) that should have been sent to the SDK router after we close the network?

If we are not okay with this - then I think we'll need to be fairly careful around what gets passed through to the router.

We chatted offline but agreed it was cleanest to leave the Router code as-is, and just drop the closed check on the server-end because we're guaranteed that the outstanding request is empty on shutdown because we empty it on Shutdown and stop sending requests after the flag is set.